Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for the WIA and RVZ disc image formats #8538

Merged
merged 36 commits into from Jun 21, 2020

Conversation

JosJuice
Copy link
Member

@JosJuice JosJuice commented Dec 28, 2019

To whoever wants to merge this: Please contact me first, so that I can bump the RVZ version number to 1.0 before the merge.

Source/Core/DiscIO/WIABlob.h Outdated Show resolved Hide resolved
Source/Core/DiscIO/WIABlob.cpp Outdated Show resolved Hide resolved
{
WIAFileReader::WIAFileReader(File::IOFile file) : m_file(std::move(file))
{
m_file.Seek(0, SEEK_SET);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems kind of odd to put the seek into the constructor, especially given it's correcting an artifact of the current calling code that executes the constructor.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So you mean that it would be better to put the seek inside Create (before the call to the constructor) instead? I was thinking it would make more sense to put it inside the constructor, since that means the constructor is making less assumptions about the calling code.

@RinMaru
Copy link

RinMaru commented Dec 28, 2019

never heard of this format what tool makes it? and does it have a gui or a bat file to use i have no clue how cmd works
Is this going to be added to dolphin as a compression method it can do itself?

@iwubcode
Copy link
Contributor

iwubcode commented Dec 28, 2019

@JosJuice - not to get too off topic but I'm curious why you chose to support this over NKit? I haven't followed either format too closely but know that many were impressed by NKit's compression ratio and apparently it even works with GCZ as well.

Regardless, looking forward to this!

@mbc07
Copy link
Contributor

mbc07 commented Dec 29, 2019

@iwubcode NKIT images (both .nkit.iso and .nkit.gcz formats) works out of the box with Dolphin, there's no special handling required*...

*technically Wii images in NKIT format doesn't work in 5.0 stable because it shipped with broken support for unencrypted Wii volumes, but that issue is fixed in current development builds

@iwubcode
Copy link
Contributor

@mbc07 - ah ok, wasn't sure what this was doing. I didn't look at the code but yeah seeing the commits, it seems like it's being used to play compressed games.

I was thinking of more of a "right click" -> compress with -> "nkit" (or wia) and assumed @JosJuice was planning something like that but I think I misread the whole thing.

@JosJuice
Copy link
Member Author

never heard of this format what tool makes it? and does it have a gui or a bat file to use i have no clue how cmd works

wit can create it. I'm not sure if there's a GUI for it.

Is this going to be added to dolphin as a compression method it can do itself?

My plan is to add the ability for Dolphin to write WIA files eventually. But I want to improve the WIA reading code more first.

@iwubcode Like mbc07 mentioned, Dolphin can already run NKit files. A design goal for NKit was to have the files be playable in Dolphin without needing to make any changes to Dolphin. Anyway, I would prefer adding writing support for WIA rather than NKit. NKit disc images are not netplay/TAS compatible with normal disc images, and there is currently a known bug with not being able to start a new game in Super Paper Mario. WIA avoids this problem (the reason NKit has these problems is the design goal I mentioned earlier), and WIA can get even better compression using bzip2 or LZMA (though that hasn't been implemented in this PR yet).

@JosJuice
Copy link
Member Author

I have added support for WIA's "purge" compression method. It's a simple way of storing runs of zeroes more efficiently.

The compression methods that remain unimplemented are bzip2, LZMA, and LZMA2. Unfortunately, adding support for them involves one of the parts of C++ development that I'm worst at: adding libraries. If someone wants to help me add the bzip2 and LZMA libraries to Dolphin (without necessarily changing anything in the WIA reading code), that would be much appreciated. Otherwise this is probably going to take a while.

@RinMaru
Copy link

RinMaru commented Dec 31, 2019

What formats you plan to support i dont have ISO wii dumps just wbfs from usbloaderGX. will this support compressing wbfs to wia?

@JosJuice
Copy link
Member Author

JosJuice commented Jan 1, 2020

I currently don't have any plans to support converting from WBFS, since it would require heuristics for determining the size of the original disc image (since WBFS doesn't store the size). But maybe it could be added if enough people think it's important. For now, I would suggest using wit for converting from WBFS to WIA.

@mbc07
Copy link
Contributor

mbc07 commented Jan 1, 2020

@RinMaru please stay on topic. This PR discussion is not a support thread.

@Starmann
Copy link

Starmann commented Jan 3, 2020

I would like NKit-making support for Dolphin because IIRC, WIA doesn't work on actual hardware

@JosJuice
Copy link
Member Author

JosJuice commented Jan 3, 2020

Dolphin is not a program that is intended to help you run GC/Wii games on the original hardware. Also, Wii NKit doesn't work on retail consoles – only GC NKit does.

@mbc07
Copy link
Contributor

mbc07 commented Jan 3, 2020

And like I mentioned earlier, NKIT images work out of the box in Dolphin, no special handling is needed...

@Starmann
Copy link

Starmann commented Jan 3, 2020

I'm aware that Dolphin supports NKIT out of the box

@JosJuice
Copy link
Member Author

JosJuice commented Jan 4, 2020

bzip2, LZMA and LZMA2 now work, but there are some pretty noticeable performance problems.

@JosJuice
Copy link
Member Author

JosJuice commented Jan 5, 2020

I've done some speed optimizations. WIA images with a chunk size of 2 MiB are now fast enough to be playable, but I'm not sure if there's a little bit of lag when the game is loading a lot of data or if that's just my computer giving me a little bit of lag for unrelated reasons. I haven't tested larger chunk sizes, but I imagine that large chunk sizes (like wit's default, 40 MiB) are going to cause some pretty noticeable lag when the game seeks on the disc.

@JosJuice
Copy link
Member Author

JosJuice commented Jan 6, 2020

I have written some documentation for the WIA format and added it to the PR, since there wasn't really any documentation for it other than source code.

@JosJuice
Copy link
Member Author

JosJuice commented Jan 7, 2020

I've fixed a problem that made the Wii partition data reading code not handle chunk sizes larger than 2 MiB correctly. Just like I theorized earlier, there are big slowdowns when the game tries to seek if you're using wit's default chunk size of 40 MiB. Purely sequential access (like when streaming a video from the disc) runs well, though.

@JosJuice
Copy link
Member Author

JosJuice commented Jun 20, 2020

To describe how to interpret the DVD thread log for anyone who wants to test: "Real time" should be lower than "emulated time including delay". "Real time including delay" should be similar to "emulated time including delay" (the former is typically a tiny bit larger). If "real time" and "real time including delay" are noticeably larger than "emulated time including delay", there is a performance problem. This all assumes that you are using 100% emulation speed.

@Anuskuss
Copy link

I don't really wanna mess with debugging and stuff, I'll leave that to you guys. I'll just convert Wario World, place in on my NAS, and see if/when the audio stutters (or not).

@Anuskuss
Copy link

Anuskuss commented Jun 20, 2020

16 MiB is the heighest I can go without stuttering. I know my case isn't the absolute worst case (USB 3.0 connected to NAS), but I think it's pretty safe to say that 2 MiB is a good default (and minimum). Also the gains are not really impressive:

bs=2048  192,660,052
bs=4096  191,653,880 -0.52 %
bs=8192  190,932,612 -0.90 %
bs=16384 190,295,344 -1.23 %
bs=32768 188,974,540 -1.91 %
bs=65536 188,073,012 -2.38 %

So again this really supports my stance:

  • Set 2 MiB as default but let the user (with a warning) increase that
  • Remove LZMA from RVZ and leave it to WIA since it's buggy and barely useful
  • Always select max compression (I did six compressions in two minutes so it really is fast enough)
  • Additionally: Remove update partition for Wii games (but restore in a seperate tool idc)

I'm all for streamlining this. The more people use the same config, the easier it is to track regressions.

@RinMaru
Copy link

RinMaru commented Jun 20, 2020

Just tested myself and yea 16MB is about as high as you can go but also very a miniscule diff in compression size
m using and external HDD so it should be good enough

@degasus
Copy link
Member

degasus commented Jun 20, 2020

@Anuskuss This kind of performance testing is useless. JosJuice has pushed a good way to actually check for the performance of the decompression. Having too large chunks is by far worse than having too small chunks. So I don't think we should increase the default without checking this INFO_LOG (you can do this in the UI).

I'm -1 on removing the update partition. One of the main goal of RVZ is to not modify the image. Feel free to use an external tool to do so, but we should not suggest this to everyone.

@RinMaru
Copy link

RinMaru commented Jun 20, 2020

@Anuskuss This kind of performance testing is useless. JosJuice has pushed a good way to actually check for the performance of the decompression. Having too large chunks is by far worse than having too small chunks. So I don't think we should increase the default without checking this INFO_LOG (you can do this in the UI).

in detail can you tell me how to get the results and what i should post?

@JosJuice
Copy link
Member Author

JosJuice commented Jun 20, 2020

in detail can you tell me how to get the results and what i should post?

Using the latest build, turn on DVDINTERFACE logging, then follow my last comment to interpret the results. Let us know whether there are any cases where the real time is noticeably slower than the emulated time.

@RinMaru
Copy link

RinMaru commented Jun 20, 2020

in detail can you tell me how to get the results and what i should post?

Using the latest build, turn on DVDINTERFACE logging, then follow my last comment to interpret the results. Let us know whether there are any cases where the real time is noticeably slower than the emulated time.

what should verbosity be set to?

@JosJuice
Copy link
Member Author

Because I changed it to NOTICE_LOG, any verbosity works.

@RinMaru
Copy link

RinMaru commented Jun 20, 2020

dolphin.log

heres a log from wario world

@JosJuice
Copy link
Member Author

Hmm... Well, there seem to be some cases where the DVD thread is being slow, though only when the calculated time is relatively low. Could you get a log for a lower block size (or plain ISO) in the same part of the game to compare? Also, what block size was this with?

@RinMaru
Copy link

RinMaru commented Jun 20, 2020

Hmm... Well, there seem to be some cases where the DVD thread is being slow, though only when the calculated time is relatively low. Could you get a log for a lower block size (or plain ISO) in the same part of the game to compare? Also, what block size was this with?

I was using RVZ,2MB,Zstd, lvl22

Heres a log from ISO
Updated
dolphin.log

@JosJuice
Copy link
Member Author

The newly uploaded log seems like it contains a copy of the old log at the beginning. At what point does the ISO testing start?

@RinMaru
Copy link

RinMaru commented Jun 20, 2020

The newly uploaded log seems like it contains a copy of the old log at the beginning. At what point does the ISO testing start?

try it again i uploaded a new one

@JosJuice
Copy link
Member Author

JosJuice commented Jun 20, 2020

Based on these logs, it does seem like RVZ has performance problems in situations where a plain ISO does not. However, the performance loss is just something like 3 ms each time, which usually is little enough that you don't notice it. Based on this, I'm reluctant to change the default block size to 2 MiB. However, I must say that I'm not entirely sure what this minor performance problem is caused by... So I'm not sure that it doesn't happen with smaller block sizes. If you want to test with 128 KiB, that would be appreciated.

@RinMaru
Copy link

RinMaru commented Jun 20, 2020

will do hold on
block size 128kb
dolphin.log

@JosJuice
Copy link
Member Author

JosJuice commented Jun 20, 2020

The 128 KiB log has much less of the problem than the 2 MiB log (though it seems like ISO is still a little bit better).

@RinMaru
Copy link

RinMaru commented Jun 20, 2020

The 128 KiB log has much less of the problem than the 2 MiB log (though it seems like ISO is still a little bit better).

should i try anything else?

@JosJuice
Copy link
Member Author

No, I have nothing else in particular that I would like to have tested for now.

@RinMaru
Copy link

RinMaru commented Jun 20, 2020

I should probably say I used the lvl 22 compression on all block sizes

@Anuskuss
Copy link

Well, I was like "if I can't feel the difference it doesn't matter" but for the sake of it I wrote a little script and went through all the block sizes:

File: 32.log
RealTime > EmulatedTimeDelay
  Amt: 0 (0.00%)
  Med: 0 μs (0.00%)
  Max: 0 μs (0.00%)

File: 64.log
RealTime > EmulatedTimeDelay
  Amt: 0 (0.00%)
  Med: 0 μs (0.00%)
  Max: 0 μs (0.00%)

File: 128.log
RealTime > EmulatedTimeDelay
  Amt: 0 (0.00%)
  Med: 0 μs (0.00%)
  Max: 0 μs (0.00%)

File: 256.log
RealTime > EmulatedTimeDelay
  Amt: 0 (0.00%)
  Med: 0 μs (0.00%)
  Max: 0 μs (0.00%)

File: 512.log
RealTime > EmulatedTimeDelay
  Amt: 0 (0.00%)
  Med: 0 μs (0.00%)
  Max: 0 μs (0.00%)

File: 1024.log
RealTime > EmulatedTimeDelay
  Amt: 1 (1.02%)
  Med: 2473 μs (6.67%)
  Max: 2473 μs (6.67%)

File: 2048.log
RealTime > EmulatedTimeDelay
  Amt: 3 (2.17%)
  Med: 1626 μs (3.15%)
  Max: 4390 μs (1463.33%)

File: 4096.log
RealTime > EmulatedTimeDelay
  Amt: 2 (1.36%)
  Med: 24878 μs (673.36%)
  Max: 45907 μs (1282.67%)

File: 8192.log
RealTime > EmulatedTimeDelay
  Amt: 6 (4.76%)
  Med: 21282 μs (41.36%)
  Max: 83280 μs (2058.33%)

File: 16384.log
RealTime > EmulatedTimeDelay
  Amt: 10 (8.13%)
  Med: 37734 μs (59.84%)
  Max: 198406 μs (5448.00%)

File: 32768.log
RealTime > EmulatedTimeDelay
  Amt: 18 (15.25%)
  Med: 29173 μs (75.07%)
  Max: 435706 μs (8471.33%)

File: 65536.log
RealTime > EmulatedTimeDelay
  Amt: 26 (21.85%)
  Med: 113130 μs (102.90%)
  Max: 678034 μs (769.29%)

I chose the median instead of the average because of those huge single spikes. Also these test were ran over the span of 15 seconds or so (one full rotation in the Wario World starting area) but it should serve as a rough insight. I can test more (especially 512 KiB since that seems to be the highest flawless) if you want to.

@degasus
Copy link
Member

degasus commented Jun 21, 2020

Thanks for the sane testing. "if I can't feel the difference it doesn't matter" is a common misunderstanding. You won't feel it as long as everything else is fast enough to catch up. But I don't want to sacrifice our performance headroom for a small percentage lower disk usage. We should also care about users with slower computers, I assume many of those images will end up on mobile devices as well.

Honestly, the 100% emulation speed might be wrong here. If your computer is able to handle 4x the emulation speed, the decompression should do so as well.

@JosJuice
Copy link
Member Author

Thanks for the testing. I have removed the temporary testing commits now. Due to the results of the testing, I will not be changing the default block size to 2 MiB.

@Tilka Tilka merged commit 9982251 into dolphin-emu:master Jun 21, 2020
@JosJuice JosJuice deleted the wia branch June 21, 2020 10:42
@Anuskuss
Copy link

@JosJuice I'm assuming 128 KiB was not chosen arbitrarily so what exactly was your motive? Is it by convention or did you do some (real world) tests on your own?

Also I can't quite figure out what block size NKit+GCZ uses (I'm presuming 16 KiB) but RVZ is beating NKit+GCZ at 32 KiB which is quite nice. I'm definitely switching to RVZ now, good job! I just wanna be sure that I'm choosing the correct block size so that I can go right ahead and convert my collection. The process takes some time though that's why I'm extra careful. I might even go as low as 32 KiB because I've tested NKit+GCZ (with its much slower deflate compression) and it manages to keep the real time lower than the emulated time in my test and 32 KiB seems to be the best choice historically (reliability > some wasted megabytes). Hopefully you can help me to clear up my doubts. At the end of the day I'm gonna pick what you think is a good default (except for the level which should always be 22 😉).

P.S. What is your stance on removing the update partition or should that be left to other tools? I might ask @Nanook what he's up to these days.

@JosJuice
Copy link
Member Author

P.S. What is your stance on removing the update partition or should that be left to other tools? I might ask @Nanook what he's up to these days.

I suppose I'm currently undecided on this.

@JosJuice I'm assuming 128 KiB was not chosen arbitrarily so what exactly was your motive? Is it by convention or did you do some (real world) tests on your own?

I explained this earlier:

I did some calculations on Dolphin's emulated disc drive timings and came to the conclusion that with 128 KiB, you should be able to handle any seek without getting performance problems if you are able to handle sequential access without performance problems, assuming that the hard drive's seek time isn't more than 10-20 ms.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet